Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimize Image Handling and Open Wikidata Media within app #6187

Conversation

Sujal-Gupta-SG
Copy link
Contributor

@Sujal-Gupta-SG Sujal-Gupta-SG commented Feb 15, 2025

Fixes #6182

Description:

  • Improved image handling by ensuring requests are only sent through Glide when an image is available.
  • Set a default icon when no image is present to prevent unnecessary network calls.
  • Implemented a feature to open Wikidata media data within app when the image is clicked.

@Sujal-Gupta-SG
Copy link
Contributor Author

I am also trying to implement the feature so that Wikidata opens in our app like the Explore section.

Until then, the raised PR will handle this by redirecting the user to the website.

Once I implement the in-app functionality, I will raise a new PR for that, as it will take some time to integrate Wikidata into the app like Explore.

@nicolas-raoul
Copy link
Member

feature so that Wikidata opens in our app like the Explore section

Would you mind creating an issue regarding this, and explain how it would work, ideally with mockup screenshots? Thanks! :-)

@Sujal-Gupta-SG
Copy link
Contributor Author

feature so that Wikidata opens in our app like the Explore section

Would you mind creating an issue regarding this, and explain how it would work, ideally with mockup screenshots? Thanks! :-)

I am currently working on this, but it is not yet complete. Once it is finished, I will create an issue and raise a PR. I will also provide a screen recording to demonstrate the functionality.

In the meantime, you can merge this PR if there are no issues with it.

Regarding the functionality, it works similarly to the Explore section: When we click on any contribution, it opens in MediaDetailPager, displaying the media details within our app.

My idea is to implement the same behavior in the Nearby section - Instead of opening the image in a browser when clicking on a green pin and then on image which appears after this branch get merged , it should open directly inside our app.

If you want me to raise the issue now, I can do so. However, if needed, I can raise it once the implementation is complete.

@Sujal-Gupta-SG Sujal-Gupta-SG changed the title Optimize Image Handling and Open Wikidata Media in Browser Optimize Image Handling and Open Wikidata Media within app Feb 17, 2025
@Sujal-Gupta-SG
Copy link
Contributor Author

Sujal-Gupta-SG commented Feb 17, 2025

Now it works like
For reviewing Please use production version
https://github.com/user-attachments/assets/61f1c92f-7649-4958-b15f-54237e3e7ccf

@nicolas-raoul
Copy link
Member

Screencast looks excellent! :-)

@nicolas-raoul
Copy link
Member

If you want me to raise the issue now, I can do so. However, if needed, I can raise it once the implementation is complete.

Now I understand better. Either is fine with me. :-)

@Sujal-Gupta-SG
Copy link
Contributor Author

I believe creating a new issue is no longer necessary since I have already merged the changes into this branch.

@nicolas-raoul
Copy link
Member

Sounds good!

@Sujal-Gupta-SG
Copy link
Contributor Author

@nicolas-raoul Could you please review this before any more PRs get merged?

@nicolas-raoul
Copy link
Member

Would you mind checking why CI is failing?

@Sujal-Gupta-SG
Copy link
Contributor Author

Sujal-Gupta-SG commented Feb 19, 2025

Would you mind checking why CI is failing?

@nicolas-raoul It may be failing because the GitHub token doesn't have access to write on PR discussions.

I have written the steps to fix this issue in the PR below:
🔗 PR #6196 - Issue Comment

Kindly check the token.

  • If it has a different name, please let me know.
  • Otherwise, this PR is working completely fine, and the only CI failure is due to the GitHub token permissions.

@nicolas-raoul
Copy link
Member

nicolas-raoul commented Feb 20, 2025

Generally working great, though I noticed a red pin item showing the image of the item I selected immediately before. Probably a race condition:

screen-20250220-235004.mp4

@nicolas-raoul
Copy link
Member

nicolas-raoul commented Feb 20, 2025

Would it be possible to replace the "refresh" icon with a "loading" icon?

You can reuse the one from the Review activity.

@Sujal-Gupta-SG
Copy link
Contributor Author

Generally working great, though I noticed a red pin item showing the image of the item I selected immediately before. Probably a race condition:

screen-20250220-235004.mp4

I have added a clear step to ensure that in every process, the previously requested image is cleared before fetching the current image. This prevents any race conditions and ensures the correct image is displayed.

…how-picture-thumbnail-' into Nearby-When-tapping-green-pin,-show-picture-thumbnail-
@Sujal-Gupta-SG
Copy link
Contributor Author

Would it be possible to replace the "refresh" icon with a "loading" icon?
You can reuse the one from the Review activity.

I think this might not be possible because the loading icon you're referring to is actually a ProgressBar:

<ProgressBar
    android:id="@+id/pb_review_image"
    android:layout_width="wrap_content"
    android:layout_height="wrap_content"
    android:layout_centerInParent="true"
    android:visibility="gone"
    tools:visibility="visible" />

Since it's part of the layout rather than a standalone icon or animation, directly replacing the refresh icon with it wouldn't work.

However, I will try:

  • Applying an animation to replicate the loading effect.
  • Creating an icon similar to that progress animation.

@nicolas-raoul
Copy link
Member

Understood for the icon, thanks! 🙂

Copy link
Member

@nicolas-raoul nicolas-raoul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Working great, thanks!

@nicolas-raoul nicolas-raoul merged commit d32ab15 into commons-app:main Feb 22, 2025
1 check failed
@Sujal-Gupta-SG Sujal-Gupta-SG deleted the Nearby-When-tapping-green-pin,-show-picture-thumbnail- branch February 22, 2025 06:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Nearby: When tapping green pin, show picture thumbnail
2 participants